Skip to content

Fix #54224 (const promotion regression) #54715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 12, 2018
Merged

Fix #54224 (const promotion regression) #54715

merged 2 commits into from
Oct 12, 2018

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Oct 1, 2018

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2018
@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk oli-obk force-pushed the nll_deref_promotion branch from 12cee74 to 670d07e Compare October 1, 2018 12:08
@TimNN
Copy link
Contributor

TimNN commented Oct 9, 2018

Ping from triage @eddyb / @rust-lang/compiler: This PR requires your review.

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 11, 2018

r? @RalfJung

@rust-highfive rust-highfive assigned RalfJung and unassigned eddyb Oct 11, 2018
@RalfJung
Copy link
Member

I am confused. #54224 says it's not a bug, and it also doesn't mention anything about this being a regression. And then the fix for an issue that something doesn't get promoted is to make more things NOT_CONST?

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 11, 2018

This is a regression introduced in #51990 (comment)

We'll want to relax this in the future, but for now we should not change behaviour

@RalfJung
Copy link
Member

Ah, so right now we accidentally accept some of the code you added in that test case?

@RalfJung
Copy link
Member

There still is a Qualif::NOT_CONST below a self.mode test at

rust/src/librustc_mir/transform/qualify_consts.rs

Line 738 in 670d07e

self.add(Qualif::NOT_CONST);
Is that correct? If yes, could you add a comment saying why this is only rejected for Fn but not for the other modes?

Same for

rust/src/librustc_mir/transform/qualify_consts.rs

Line 762 in 670d07e

self.add(Qualif::NOT_CONST);

@RalfJung RalfJung mentioned this pull request Oct 12, 2018
Closed
@RalfJung
Copy link
Member

Oh I see, the difference is that in the other cases we always emit a feature gate in the else. That's what went wrong here. Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 12, 2018

📌 Commit 76f8a90 has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2018
@bors
Copy link
Collaborator

bors commented Oct 12, 2018

⌛ Testing commit 76f8a90 with merge e9e27e6...

bors added a commit that referenced this pull request Oct 12, 2018
@bors
Auto merge of #54715 - oli-obk:nll_deref_promotion, r=RalfJung
e9e27e6 
Fix #54224 (const promotion regression)

r? @eddyb
@bors
Copy link
Collaborator

bors commented Oct 12, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: RalfJung
Pushing e9e27e6 to master...

@bors bors merged commit 76f8a90 into rust-lang:master Oct 12, 2018
@oli-obk oli-obk deleted the nll_deref_promotion branch June 15, 2020 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@RalfJung RalfJung

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@oli-obk @rust-highfive @TimNN @RalfJung @bors @eddyb